-
Notifications
You must be signed in to change notification settings - Fork 81
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add SYCL_CTS_ENABLE_EXT_ONEAPI_TESTS CMake option #771
Add SYCL_CTS_ENABLE_EXT_ONEAPI_TESTS CMake option #771
Conversation
This commit adds the SYCL_CTS_ENABLE_EXT_ONEAPI_TESTS as a shortcut for enabling all oneapi extension tests. It will only overwrite the other options if turned ON. In the OFF state the option has no effect. Signed-off-by: Larsen, Steffen <[email protected]>
Can we make Let's consider this set of options: SYCL_CTS_ENABLE_EXT_ONEAPI_TESTS=ON SYCL_CTS_ENABLE_EXT_ONEAPI_MEMCPY2D_TESTS=OFF. With the current patch the second option is ignored. |
Perhaps the semantics could be that |
Couldn't this be achieved by simply passing add_cts_option(SYCL_CTS_ENABLE_EXT_ONEAPI_SUB_GROUP_MASK_TESTS
"Enable extension oneAPI sub_group_mask tests" ${SYCL_CTS_ENABLE_EXT_ONEAPI_TESTS}) |
That was my initial thought too, but the downside to it is that the other options won't change if you set The use-case in mind for this option was to allow for enabling all extension tests in the given group (here |
@MathiasMagnus any feedback on this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Functionally looks good, just one perf/stylistic change.
Fair, but couldn't the same argument be made in reverse then? I.e., once
Yes that makes sense. In that scenario changing the configuration later on wouldn't be an issue though. The CI setup only has to make sure that |
needs another week |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The solution sounds good for the envisioned use case.
…_all_oneapi_tests_option
Signed-off-by: Larsen, Steffen <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ship it!
This commit adds the SYCL_CTS_ENABLE_EXT_ONEAPI_TESTS as a shortcut for enabling all oneapi extension tests. It will only overwrite the other options if turned ON. In the OFF state the option has no effect.